Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Rust implementation for Python client's update_run() method. #1314

Merged
merged 2 commits into from
Dec 9, 2024

Conversation

obi1kenobi
Copy link
Collaborator

Implement the update_run() client method in the Rust PyO3 bindings, and call it from the Python client if the Rust bindings are enabled.

@@ -47,8 +47,7 @@ impl BlockingTracingClient {
Ok(Self { client: Arc::from(client) })
}

// N.B.: We use `Py<Self>` so that we don't hold the GIL while running this method.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GIL info here was outdated, so I removed it. The current implementation drops the GIL explicitly by calling allow_threads().

}

Ok(Some(attachments))
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just moved this function down, to keep all the type definitions next to each other in the file. Otherwise it remained unchanged.

@@ -196,6 +213,45 @@ impl FromPyObject<'_> for RunCommon {
}
}

fn extract_attachments(value: &Bound<'_, PyAny>) -> PyResult<Option<Vec<Attachment>>> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is unchanged, just moved farther down in the same file.

Comment on lines +245 to +246
ref_name: key.to_string(),
filename: key.to_string(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's the ref_name

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The attachments dict is name -> (mime_type, data), so is using the key for both ref_name and filename the right thing to do?

Copy link
Contributor

@agola11 agola11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a comment but otherwise lgtm

@obi1kenobi obi1kenobi merged commit e0229b9 into main Dec 9, 2024
20 checks passed
@obi1kenobi obi1kenobi deleted the pg/python-update-run branch December 9, 2024 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants